fix(#186): maestro-interop — runner-leak reacquire + CDP re-pin, runFlow allowlist, route-drift detection#188
Merged
Merged
Conversation
#186 P0) The maestro-mcp <-> rn-dev-agent driver conflict: both drive the same iOS device via XCUITest, so after a maestro_run the next device_snapshot saw the AgentDeviceRunner sentinel, did a ~44s full relaunch, then cdp_navigation_state hit a ~47s STALE_TARGET — ~91s lost per occurrence. Both the daemon-leak and the maestro-eviction case surface as the SAME sentinel, so instead of trying to distinguish them (the brainstormed heuristic was contradicted by the field telemetry, which labels the maestro case "agent-device-runner-leak"), this adds a strictly-additive cheap recovery tier that helps regardless of cause and falls back to today's behavior: - runner-leak-recovery.ts: new Tier-0 `reacquire` (before attach-only/relaunch). Optional `reacquire` dep; when it clears the sentinel, recovery returns tier:'reacquire' WITHOUT closing the session or relaunching the app. - device-session.ts: wires reacquireIosTargetApp = stopFastRunner -> simctl launch booted <appId> (re-foreground, displacing the foreign XCUITest session) -> ensureFastRunner(deviceId, appId). Mirrors repair-action's bringTargetAppToForeground (which already stops the runner first SPECIFICALLY because "a prior maestro_run is still alive"), kept local per that file's tight-dependency-surface rationale. Only wired with full iOS context (appId+deviceId); otherwise omitted so recovery falls back cleanly. - CDP re-pin: device recovery sets a process-scoped markCdpStale() flag; withConnection consumes it before the next cdp_* handler and proactively recoverFromStaleTarget()s (no-op if actually fresh) — converting the 47s catch-path STALE_TARGET into a fast pre-handler re-pin. - device-interact.ts: widened recoveredTier consumers to RecoveryTier. Tests: gh-186-reacquire-tier.test.js (reacquire clears sentinel -> no relaunch; fall-through reacquire->attach->relaunch order; no-dep unchanged; markCdpStale read-and-clear). Full suite 1567 pass; tsc clean. Note: efficacy against a *held* maestro XCUITest session is unverified without a live maestro+rn-dev-agent repro; this change is strictly additive (worst case = today's behavior + a few seconds before the existing relaunch), so it cannot regress. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…P1)
cdp_run_action hard-failed any saved action using `runFlow` ("Command not in
allowlist") — commonly used for conditional dialog handling (deep-link "Open
in", Expo dev-client picker) — forcing a manual fallback to maestro-mcp.
maestro-validator.ts now allows `runFlow` with special handling:
- validateRunFlowValue: inline `commands` are validated as COMMANDS (recursive
allowlist check), NOT via generic validateValue — which would let a nested
`runScript` slip through as a plain scalar.
- expandRunFlows: {file} refs are securely resolved (relative-only, no `..`,
.yaml/.yml only, realpath-contained within flowRoot — defeats symlink escape,
depth cap 5, cycle detection) and EXPANDED INLINE so the flow written to /tmp
has no remaining file refs (a relative ref would otherwise break there). A
{file,when} becomes an inline conditional {when, commands} (semantics
preserved); without `when` the sub-flow commands are spliced flat. A {file}
ref with no flowRoot context is rejected.
- maestro-run.ts threads flowDir/flowRoot = dirname(flowPath) when running a
saved flow file; inline YAML keeps file refs rejected.
Tests: gh-186-runflow-validator.test.js (inline allow + nested-denied reject;
file resolve+inline; traversal/absolute/symlink-escape/bad-ext/cycle/depth/
no-root rejects). Full suite 1577 pass; tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n inserted screen (#186 P1) cdp_run_action auto-repair only handled SELECTOR_NOT_FOUND (stale selectors). A saved action that assumed HomeAddress -> PhoneNumber but hit a newly-inserted CouponCode screen would have its selector failure fuzzy-repaired — wrong, since the flow changed shape, not a testID. - reusable-action.ts: new M7 `expectedRouteSequence` field (parse/serialize), and ROUTE_DRIFT added to ActionFailureCode + AutoRepairRefusedReason. types.ts: ROUTE_DRIFT ToolErrorCode. - nav-graph/route-sequence.ts (pure, tested): validateRouteSequenceAgainstGraph (pre-flight definite-mismatch, conservative — no-ops on an empty/unknown graph) and classifyRouteDriftAfterFailure (live route off the expected sequence = drift). - run-action.ts: after a SELECTOR_NOT_FOUND, if the action recorded an expected sequence and the live route (bounded best-effort fetch via injectable getLiveRoute) is off it, reclassify as ROUTE_DRIFT, persist it, and SKIP repair with an actionable "re-record the action" message. Tests: gh-186-route-sequence (helper truth tables) + gh-186-route-drift-runaction (off-sequence -> ROUTE_DRIFT + repair skipped; on-sequence -> repair still runs; no-sequence -> inert). Full suite 1587 pass; tsc clean. Remaining activation (separate slice, logic+contract complete here): populate expectedRouteSequence at cdp_record_test_save_as_action time from nav events, and wire a real CDP-backed getLiveRoute in index.ts (the default is a safe no-op). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Lykhoyda
added a commit
that referenced
this pull request
May 30, 2026
…189) (#190) #188 merged the runFlow allowlist + non-destructive reacquire tier + route-drift detection to main with NO version bump, so the fix was undeliverable to marketplace installs and got re-reported as #189 on 0.44.44. Bump via changesets: rn-dev-agent-plugin 0.44.44 -> 0.44.45, rn-dev-agent-cdp 0.38.39 -> 0.38.40. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced May 30, 2026
Lykhoyda
added a commit
that referenced
this pull request
May 30, 2026
…em) (#192) #188 merged a behavior fix (runFlow allowlist) to main with NO version bump, so it was undeliverable to marketplace installs and got re-reported as #189. Add a PR-only CI job that fails when scripts/cdp-bridge/src/ changes without a changeset — making the missing-bump class impossible rather than merely discouraged. - scripts/require-changeset.sh: the guard (seams: CHANGED_FILES, REPO_ROOT, BASE_REF) - scripts/test/require-changeset.test.sh: 4-case TDD regression test - ci.yml: PR-only require-changeset job + run the guard unit test in the build job Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced May 31, 2026
Lykhoyda
added a commit
that referenced
this pull request
Jun 4, 2026
…ey, label scheme (#220) Deep-research deliverable: apply the Kano model to prioritize the 16 open issues. - 2026-06-04-kano-model-research-report.md — cited report (origins, 5 categories, functional/dysfunctional eval table, SI/DI, no-survey proxy adaptation, critiques, OSS triage). Includes a provenance caveat: the deep-research verify stage hit a StructuredOutput tooling failure (all claims defaulted to "refuted" via 0-0 abstain), so cited facts are manually vetted (canonical Kano), not machine-verified. - kano-backlog-categorization.md — 16 issues categorized + Kano-gated priority waves; factors in that #202/#186/#201 largely shipped (#218/#188). - kano-survey-template.md — functional/dysfunctional questionnaire tailored to the plugin. - kano-label-scheme-and-triage.md — kano:* + effort:* labels, gh commands, monthly cadence. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
5 tasks
Lykhoyda
added a commit
that referenced
this pull request
Jun 5, 2026
…r warning (#186) (#221) * docs(202): Phase 3 design — 3-layer contract + proactive foreign-runner warning (#186) Final #202 phase. Formalizes the §2 three-layer device-control contract (L1 introspection shared / L2 interaction shared / L3 flow exclusive) in docs, and adds a PROACTIVE foreign-runner detection warning. Reconciliation (§0): the reactive #186 fix — runner-leak reacquire + CDP re-pin (items 2+7), runFlow allowlist (1), route-drift (3) — already shipped in #188. Phase 3 does NOT rebuild it. Scope trimmed to: (A) write down the §2 contract + the #188 handoff behavior; (B) add detectIosExternalRunner + an informational FOREIGN_RUNNER_ACTIVE warning on device_snapshot open, gated on no local flow lease (else the WDA is ours). Items 4/5/6 deferred to follow-ups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(202): Phase 3 implementation plan — foreign-runner detection + contract docs 5 TDD tasks: detectIosExternalRunner, foreignRunnerNotice (flow-lease gate), device-session wiring (informational FOREIGN_RUNNER_ACTIVE), the §2 contract docs (CLAUDE.md + architecture.mdx + maestro-interop handoff page), changeset. Scoped against #188 (no recovery rebuild). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(202): apply multi-LLM plan-review + live-ps-validation findings (Phase 3) Amendments from the Gemini+Codex+Claude plan review and a live `ps` validation (Task 0) run against a booted iPhone 17 Pro while a real maestro flow drove it: - Matcher corrected: maestro's iOS driver is `maestro-driver-iosUITests-Runner`, NOT WebDriverAgent (kept only as a secondary token). - UDID-scoping made mandatory: the idle maestro-mcp server carries no UDID, so an unscoped scan false-positives; detectIosExternalRunner gains a `udid` param and `line.includes(udid)` filter. New tests: different-sim ignored, idle server ignored, our RnFastRunner excluded. - Wiring: thread deviceId as udid, add RN_IOS_FOREIGN_WARN opt-out, surface detector latency in meta.timings_ms. - Teardown race documented (our own maestro driver shares the name); arbiter suppression-window deferred since action=open is not a post-flow hot path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(#202): detectIosExternalRunner + foreignRunnerNotice — UDID-scoped iOS foreign-runner detection (Phase 3) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(#202): surface FOREIGN_RUNNER_ACTIVE on iOS device-session open (Phase 3) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#202): formalize three-layer device-control contract + maestro-mcp handoff (Phase 3) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#202): changeset for Phase 3 foreign-runner contract + warning Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the high-value items of #186 (rn-dev-agent ↔ maestro-mcp iOS interop field report). Plan was cross-verified via a Gemini+Codex+Opus brainstorm. Full suite 1587 pass; tsc clean; P0 smoke-passed on the live Bridgeless app. All 3 commits GPG-signed.
P0 — driver conflict (#186 items 2 + 7), the ~91s cascade
Both tools drive the same iOS device via XCUITest; after a
maestro_runthe nextdevice_snapshotsaw the AgentDeviceRunner sentinel → ~44s full relaunch → thencdp_navigation_statehit a ~47s STALE_TARGET.Both the daemon-leak and the maestro-eviction case surface as the same sentinel, so instead of trying to distinguish them (the brainstorm's
!isSentinelheuristic was contradicted by the field telemetry, which labels the maestro case "agent-device-runner-leak"), a strictly-additive cheap recovery tier helps regardless of cause:runner-leak-recovery.ts: new Tier-0reacquirebefore attach-only/relaunch. When it clears the sentinel, recovery returnstier:'reacquire'with no relaunch.device-session.ts:reacquireIosTargetApp=stopFastRunner→simctl launch booted(re-foreground, displacing the foreign session) →ensureFastRunner— mirrorsrepair-action'sbringTargetAppToForeground(which already stops the runner first because "a prior maestro_run is still alive").markCdpStale()flag;withConnectionconsumes it and proactivelyrecoverFromStaleTargets (no-op when fresh) — converting the 47s catch-path timeout into a fast pre-handler re-pin.P1 — runFlow allowlist (#186 item 1)
cdp_run_actionhard-failed any saved action usingrunFlow("Command not in allowlist"). Now allowed with secure inline-at-validation: nestedcommandsget command-level allowlist checks;{file}refs are resolved (relative-only, no.., .yaml only, realpath-contained inflowRoot, depth-cap 5, cycle-reject) and expanded inline so the flow written to /tmp has no file refs.{file,when}→ inline{when, commands}(semantics preserved).P1 — route-drift detection (#186 item 3)
Auto-repair only handled
SELECTOR_NOT_FOUND. A saved action that hit an inserted screen (the report's CouponCode between HomeAddress→PhoneNumber) would get its selector failure fuzzy-repaired — wrong. Now: M7expectedRouteSequencefield + newROUTE_DRIFTcode (3 enums) + purenav-graph/route-sequence.tshelpers;run-actionreclassifies aSELECTOR_NOT_FOUNDasROUTE_DRIFT(and skips repair) when the live route is off the expected sequence.Tests
gh-186-reacquire-tier,gh-186-runflow-validator,gh-186-route-sequence,gh-186-route-drift-runaction(+ markCdpStale flag). Pure helpers tested directly (no socket), run-action via the DI seam.Not in this PR (tracked follow-ups)
expectedRouteSequenceatcdp_record_test_save_as_actiontime (from nav events) + wire a real CDP-backedgetLiveRouteinindex.ts(default is a safe no-op). The logic + contract are complete here; this feeds it real data.🤖 Generated with Claude Code